prefer HEAD to master in update tasks - #1502
Merged
Merged
Conversation
Contributor
When running our nightly book update job, we assume the default branch for the progit2 repo (and any translation repo) is "master". This may be changing in the future. Best case that would cause us to stop getting updates. Worst case it may actually cause subtle bugs, since we pull the tree sha by asking for "HEAD". Note that we can't just swap out "HEAD" here. GitHub's refs API expects us to ask for "heads/foo" or "tags/foo", and passing "HEAD" seems to imply "heads/HEAD". I didn't check if that's Octokit being clever, or is how GitHub's API behaves, but either way it doesn't work. We can hack around it by using the "commit" endpoint, which takes any resolvable name. And since "HEAD" always has to point to a commit (by Git's rules), we're in no danger of being confused by another object type. It does get us a bit of extra information we don't care about (we just need the sha, not the actual commit message), but the extra cost should be negligible in practice.
When we index book content, we use the sha1 of the HEAD commit as a cache key to see whether anything has been updated. But we fetch the tree content by asking for HEAD separately. It's unlikely, but the repository could be updated between the two, meaning we'd store a mis-matched cache key (and because we get the tree first, we'd be stuck with the old data under the new key, and thus wouldn't even notice on the next run). We can fix this by getting the commit first, and then using the tree sha1 it mentions, which is atomic. This is easy now that we're grabbing the whole commit (and this also matches how we do documentation indexing).
When there are no tags, we look for documentation in the hard-coded master branch. This is unlikely for git.git, but presumably triggers for translation repos. We should be more flexible and just use HEAD in this case, which will do the right thing if the translation repos switch their default branch. Note that we don't to do anything clever here like we did for the "book2" case. We're already using the commit API for the remote index, so we're free to just look up the name "HEAD". And likewise the local case is just running Git commands, so that name is fine.
Member
Author
|
Thanks, fixed! I had run this locally when I wrote it yesterday, but I just double-checked by running the tasks manually on the production site, too. Everything seems good. |
15 tasks
Member
Author
...and this wasn't quite true. It did build everything correctly, but it broke the commit-sha caching in a way that would blow up next time it tried to run. I pushed the fix straight to master (f945dfa) and am re-running the book task manually. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When we fetch book and manpage data from other repositories, we hard-code
masteras the branch name to use. Let's switch instead to the default-branchHEAD, which will do the right thing if those repositories ever change their default branch (e.g., as discussed in #1501).This isn't quite just
s/master/HEAD/because of the way GitHub's API works. See 0029905 for details. On the plus side, that change helped me identify and fix a possible race condition in the update code (fixed in cee3bf1).